-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Overhaul vendoring/linking features #74
Conversation
This reverts commit ef6ca89.
The make_libelf() and make_zlib() function are not used unless the "vendored" feature is enabled, but there is little benefit in not even defining them in that case. On the contrary: it means that we may miss out on warnings or clippy suggestions if the corresponding feature is not enabled. Let's just use a runtime feature check to guard invocation, but have the code be present unconditionally. Signed-off-by: Daniel Müller <deso@posteo.net>
Address some issues reported by clippy. Signed-off-by: Daniel Müller <deso@posteo.net>
Similar to what we do for building the other libraries, factor out a function, make_libbpf(), dedicated to building libbpf. Signed-off-by: Daniel Müller <deso@posteo.net>
With an upcoming change the novendor feature will be deprecated and should no longer be used. It will be replaced by building the library without any features. To express that in CI, switch from having a "features" argument to a more general "args", which will simply hold --no-default-features for the case where no features are desired (but for now no semantic change is made). Signed-off-by: Daniel Müller <deso@posteo.net>
We probably should suppress the output of the commands we run when we check for their existence, as it's vastly confusing to see nonsense such as ``` autoreconf: error: 'configure.ac' is required make: *** No targets specified and no makefile found. Stop. Please specify at least one package name on the command line. autopoint: *** Missing configure.in or configure.ac, please cd to your package first. autopoint: *** Stop. <stdin>:1: premature EOF bison: missing operand Try 'bison --help' for more information. Usage: gawk [POSIX or GNU style options] -f progfile [--] file ... Usage: gawk [POSIX or GNU style options] [--] 'program' file ... POSIX options: GNU long options: (standard) -f progfile --file=progfile -F fs --field-separator=fs -v var=val --assign=var=val [...] ``` show up on virtually every build script failure. Redirect stdout & stderr to /dev/null to prevent this noise from showing up. Signed-off-by: Daniel Müller <deso@posteo.net>
Uhm, let me figure out the CI stuff. |
We need to re-run the build script when the LD_LIBRARY_PATH environment variable changes, or we may end up with a build not picking up any of the libraries correctly. Signed-off-by: Daniel Müller <deso@posteo.net>
This change overhauls the vendoring & static linking features that the library exposes. Please refer to the larger discussion [0] for additional context, but in short: we introduce separate features for vendoring/linking each of the three system library dependencies: libbpf, libelf, zlib. "static" and "vendored" meta-features are still available, which apply to all three libraries in unison. The remaining dependencies are expressed declaratively via dependent features. E.g., because zlib is only a dependency of libbpf (and not a direct one), linking it statically implies linking libbpf statically. In the future, this design would make it possible to enable additional configurations. For example, currently vendoring any library implies linking it statically, because we only build the static version. This is more of a simplification than a strict requirement and if needed, we could support dynamic linking when using a vendored copy. We could alternatively move to a model where we err out on combinations that make little sense/are risky/whatever. Doing so could be beneficial if we ever were to loosen some of those dependencies down the line to minimize chance of breakage. I am unsure how likely that really is or whether it would be cause for concern, and I generally like the declarative nature of "this feature depends on this other" in Cargo.toml, but we could change that if we feel strongly. The default features mirror the previous default and no behavior change should occur. The existing novendor feature is kept but deprecated and should be removed in the future (a warning will be printed as part of the build). It was certainly one of the main causes of confusion where novendor and vendored could co-exist and it was hard to understand what would or wouldn't happen. In the new world, users are advised to simply build with all features disabled. I tested everything on a binary depending on libbpf-sys with various features enabled and spot checked the expected dynamic library dependencies. We could enshrine that as a CI step, but given that this logic is expected to change infrequently I didn't go down that road. [0] libbpf#64 (comment) Closes: libbpf#70 Signed-off-by: Daniel Müller <deso@posteo.net>
8e159d6
to
095e7f9
Compare
@mmullin-halcyon I would think that in combination with Alex' suggestion these changes should be able to support your use case, but I'd recommend you give them a try (and/or review). |
I will look at this ASAP (probably tomorrow evening). |
One more meta-comment: the current proposal enables dependent features as necessary. E.g., linking |
095e7f9
to
c4bc391
Compare
I am having a strange problem. using I compile with the following command
note: vendored for my binary is turning on static and vendored on libbpf-sys My end binary is no longer static against libc.
From the log of libbpf-sys
I can't see anywhere in the code which should cause this problem. Please give me some more time before submitting to sus-out what is going on. |
OK, all clear. TL;DR something on my end was amiss. Not sure what I did, but I did some Now let me actually review the code :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not test on AARCH64, but I suspect that the libelf static compile kludge on that platform is still needed.
c4bc391
to
97a32ea
Compare
LGTM |
.github/workflows/ci.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fluent in Github CI – does the "matrix" now test all the various feature combinations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only an adjustment of the combinations we already build. That's what I tried to convey with
I tested everything on a binary depending on libbpf-sys with various
features enabled and spot checked the expected dynamic library
dependencies. We could enshrine that as a CI step, but given that this
logic is expected to change infrequently I didn't go down that road.
Do let me know if you want to have all possible combinations tested in CI and to what degree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's okay, I was just trying to make sure I understood what was changing.
This is awesome! Left some questions/comments.
I like it too, I'm fine with it the way it is. |
97a32ea
to
eac7ce4
Compare
I'm happy if y'all are happy. Ready to merge? |
Ready from my side, thanks! |
Hi @alexforster, would you be able to cut a release containing the changes and the |
Please see #64 for a discussion and motivation and check individual commits for change descriptions.